-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Porting io/fs package #1323
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1323 +/- ##
==========================================
- Coverage 55.91% 55.09% -0.82%
==========================================
Files 420 421 +1
Lines 65415 66441 +1026
==========================================
+ Hits 36578 36608 +30
- Misses 25977 26957 +980
- Partials 2860 2876 +16 ☔ View full report in Codecov by Sentry. |
|
✅ Deploy Preview for gno-docs2 canceled.
|
// func (d dirInfo) String() string { | ||
// | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the comments from the original, as well as the copyright header.
// if err := fstest.TestFS(myFS, "file/that/should/be/present"); err != nil { | ||
// t.Fatal(err) | ||
// } | ||
// func TestFS(fsys fs.FS, expected ...string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed?
|
||
// checkDir checks the directory dir, which is expected to exist | ||
// (it is either the root or was found in a directory listing with IsDir true). | ||
// func (t *fsTester) checkDir(dir string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also why removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one needs to have commented functions uncommented, or if they rely on features we don't support then they need an appropriate // XXX
comment
as well as testfs_test.go, please :)
@thehowl First of all, thanks for the detailed review as always. It's been quite some time since I add this, and there are many parts I simply marked "no" and commented out. So, It's challenging to make immediate corrections. Thus, I plan to revert to a "draft" and make comprehensive changes :) |
Description
Note:
sort.Slice
is not supported yet (this method hasinternal/reflectlite
as an dependency). So, I've temporarily filled in the blanks with a basic sorting algorithm - like Quick Sort - which will need to be fixed later.related with: #1267